fix(core-tools): resolve defensive path resolution for at-reference files and fix macOS tests#28053
Conversation
|
📊 PR Size: size/XL
|
🛑 Action Required: Evaluation ApprovalSteering changes have been detected in this PR. To prevent regressions, a maintainer must approve the evaluation run before this PR can be merged. Maintainers:
Once approved, the evaluation results will be posted here automatically. |
|
/gemini review |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a robust, defensive path resolution mechanism to handle Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces defensive path resolution and sanitization across several tools (EditTool, ReadFileTool, WriteFileTool, and pathCorrector) by implementing and utilizing resolveDefensiveToolPath and resolveToRealPath. This helps handle LLM-generated paths safely, stripping null bytes and reference prefixes (like @), and resolving symlinks. Corresponding tests have been updated or added to verify these behaviors. The reviewer pointed out a potential vulnerability in packages/core/src/tools/edit.ts where a fallback branch for absolute paths could leak unsanitized null bytes if resolveToRealPath throws an error, and suggested sanitizing the path before fallback.
There was a problem hiding this comment.
Code Review
This pull request introduces defensive path resolution and sanitization (such as stripping null bytes and handling '@' prefixes) across the EditTool, ReadFileTool, and WriteFileTool to prevent path traversal and ensure safe workspace access. The review feedback highlights a critical edge case where paths like @/ or @\ can resolve to the workspace root, and points out an inconsistency in stripping the @ prefix when creating new directories, which could lead to the accidental creation of literal @-prefixed folders. Suggestions were provided to consistently strip the prefix and update the corresponding tests.
85be852 to
97a24ce
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces defensive path resolution and sanitization across several tools (EditTool, ReadFileTool, WriteFileTool) and utilities. It implements resolveDefensiveToolPath to strip null bytes and handle @ reference prefixes safely, and resolves paths to their real paths using resolveToRealPath to prevent path traversal vulnerabilities and handle symlinks gracefully. The review feedback highlights that in write-file.test.ts, rootDir and plansDir are declared as module-level variables with static paths, which can cause race conditions and test interference during concurrent test execution. The reviewer suggests initializing these variables with unique temporary directories inside beforeEach to ensure proper test isolation.
c6040e7 to
115f7ba
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces defensive path resolution and sanitization across various tools (Edit, ReadFile, WriteFile) to prevent path traversal and null byte injection vulnerabilities, notably by implementing resolveDefensiveToolPath and resolving paths to their real paths. It also adds comprehensive tests for at-reference path resolution, null byte sanitization, and symlink loops. The feedback highlights an inconsistency in packages/core/src/tools/write-file.ts where absolute paths are not sanitized using resolveDefensiveToolPath in getCorrectedFileContent, and suggests simplifying this by calling the sanitization function directly.
b07c820 to
bd806c7
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces defensive path resolution and sanitization across file tools (such as edit, read, and write tools) to prevent path traversal, null byte injection, and accidental creation of literal '@'-prefixed directories. The feedback highlights several locations in plan-mode resolution within edit.ts and write-file.ts where raw file paths containing null bytes could still cause resolveToRealPath to throw errors, recommending that these paths be sanitized before resolving the plan path.
b0d8d21 to
0db931a
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces defensive path resolution and sanitization across several tools (Edit, ReadFile, WriteFile) to strip null bytes and handle '@' reference prefixes safely, accompanied by comprehensive test coverage. A critical security feedback was provided regarding a potential path traversal vulnerability in write-file.ts where absolute paths in plan mode bypass resolveAndValidatePlanPath, potentially allowing writes outside the plans directory.
Note: Security Review did not run due to the size of the PR.
e240ce8 to
0db931a
Compare
…iles and fix macOS tests
0db931a to
2cae930
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces defensive path resolution and sanitization across core tools, including EditTool, ReadFileTool, and WriteFileTool. It implements a new utility, resolveDefensiveToolPath, to strip null bytes and safely handle @-prefixed reference paths, preventing accidental directory creation. Tests have been updated to use real paths and expanded to cover these new path resolution behaviors. No review comments were provided, so I have no feedback to address.
Summary
This PR implements a comprehensive, defensive path resolution fix for a critical production bug where filesystem tools (
read_file,replace,write_file) fail with a "File not found" error when the model passes a path prefixed with@(e.g.,@policies/new-policies.txt).Additionally, this PR resolves macOS-specific test failures in
EditToolandWriteFileToolcaused by path resolution mismatches (due to the/var->/private/varsymlink on macOS), sanitizes file paths by stripping null bytes to prevent potential crash vulnerabilities, and enhances test stability by generating unique session IDs intrackerTools.test.ts.Furthermore, this PR incorporates several critical security and robustness enhancements requested during code review, including consistent
@prefix stripping, plan-mode boundary enforcement, and test isolation improvements.Details
We implemented a centralized, defensive path resolution utility and applied targeted, platform-aware fixes to align the test environment with the production path resolution behavior:
Defensive Path Resolution (
packages/core/src/utils/paths.ts):resolveDefensiveToolPathto strip leading@reference prefixes if the literal path does not exist but the stripped path does.@Prefix Stripping: Updated the logic to consistently strip the@prefix when creating new files/directories (unless a literal@-prefixed directory already exists on disk), preventing the accidental creation of literal@-prefixed folders (e.g.,@src,@policies).@/and@\prefixes to always strip the@and leading slashes, ensuring that common path aliases are resolved correctly even when the target directory does not exist yet.\0) at the very beginning ofresolveDefensiveToolPathto prevent potential crash vulnerabilities (TypeError) in downstream synchronous file system operations.Tool Integration & Hardening:
ReadFileTool(packages/core/src/tools/read-file.ts): AppliedresolveDefensiveToolPathin both the constructor andvalidateToolParamValues.WriteFileTool(packages/core/src/tools/write-file.ts):resolveDefensiveToolPathin the constructor,validateToolParamValues, andgetCorrectedFileContent(simplifying and hardening the logic by calling it directly for all paths, including absolute paths).path.isAbsolutecheck ingetCorrectedFileContentto ensure that absolute paths in plan mode do not bypassresolveAndValidatePlanPath, strictly restricting the agent to the plans directory.resolveToRealPathfrom throwing errors.EditTool(packages/core/src/tools/edit.ts):resolveDefensiveToolPathin the constructor,validateToolParamValues, andgetModifyContext.resolveToRealPathfrom throwing errors.packages/core/src/utils/pathCorrector.ts): AppliedresolveDefensiveToolPathat the beginning ofcorrectPath.macOS Test Fixes & Test Isolation:
packages/core/src/test-utils/mockWorkspaceContext.ts): UpdatedcreateMockWorkspaceContextto resolve all input directories to their real paths usingfs.realpathSync. This ensures that on macOS, any directory starting with/var/...is correctly resolved to/private/var/..., matching the behavior of the production code and fixing 30 failures inedit.test.tsand boundary failures inwrite-file.test.ts.packages/core/src/tools/write-file.test.ts):rootDirandplansDirwithout static global initializers and initialized them with unique temporary directories usingfs.mkdtempSyncinsidebeforeEachto prevent race conditions and test interference during concurrent test execution.resolveToRealPathbefore asserting.packages/core/src/tools/edit.test.ts):tempDirto its real path usingfs.realpathSyncinbeforeEachto ensure all generated paths are fully resolved, fixing all remaining macOS path mismatches.Test Stability:
packages/core/src/tools/trackerTools.test.ts): Replaced the hardcodedsessionId: 'test-session'with a dynamically generated unique session ID usingMath.random()to prevent test session leakage and concurrency issues.Consolidated Test Suite (
packages/core/src/tools/at-reference-resolution.test.ts):Build Script Robustness (
scripts/build.js):npx --no-install npm-run-allto force using the locally installed version and bypass registry/network calls, making the build script robust in corporate environments (Corp Airlock).Related Issues
How to Validate
Step 1: Run the modified test suites on macOS
Run the following commands on a macOS machine to verify that all tests pass successfully:
Expected Output: All 125+ tests pass successfully with zero failures.
Step 2: Run the test suites on Linux/Windows (Regression Check)
Run the same commands on Linux or Windows to ensure zero regressions:
Expected Output: All 63 tests pass successfully with zero failures.
Step 3: Run linting and type checking
Verify that the codebase remains fully compliant with the project's engineering standards:
npm run lint && npm run typecheckExpected Output: Zero linting or type-checking errors.
Pre-Merge Checklist